Feature/inequality operator#4454
Conversation
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughAdds Changes
Benchmark runner refactor and rollup cosmetic fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/test-data/tests-unit/css-guards/css-guards.less (1)
104-115: ⚡ Quick winAdd one fixture that covers non-comparable inequality.
The new cases validate comparable
!=paths, but they don’t exercise the non-comparable branch that should still evaluate!=as true. Add one guard likewhen (foo != 1)and assert it emits CSS.Suggested fixture addition
.inequality-test-4 when (`@b` != 2) { color: red; } + +.inequality-test-5 when (foo != 1) { + color: green; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/test-data/tests-unit/css-guards/css-guards.less` around lines 104 - 115, Add a new test fixture after the existing inequality-test cases that validates non-comparable inequality evaluation. Create a guard like when(foo != 1) where foo is an undefined or non-comparable variable, and include a CSS rule (such as color: green;) that should be emitted when the guard evaluates to true. This test should demonstrate that the non-comparable branch in the inequality operator evaluation correctly returns true when comparing incompatible types or undefined values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/less/benchmark/benchmark-runner.js`:
- Around line 46-47: The compiler discovery condition at line 46 currently uses
OR logic to accept compilers that have either `render` or `parse`, but the
`runOnce()` function always calls `less.render` at line 93. Change the condition
from `(less.render || less.parse)` to `(less.render && less.parse)` to require
both methods to be present, preventing runtime crashes when a parse-only
compiler is selected.
---
Nitpick comments:
In `@packages/test-data/tests-unit/css-guards/css-guards.less`:
- Around line 104-115: Add a new test fixture after the existing inequality-test
cases that validates non-comparable inequality evaluation. Create a guard like
when(foo != 1) where foo is an undefined or non-comparable variable, and include
a CSS rule (such as color: green;) that should be emitted when the guard
evaluates to true. This test should demonstrate that the non-comparable branch
in the inequality operator evaluation correctly returns true when comparing
incompatible types or undefined values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01e6e179-62bd-4278-9565-48b010ac24d4
📒 Files selected for processing (6)
packages/less/benchmark/benchmark-runner.jspackages/less/build/rollup.jspackages/less/lib/less/parser/parser.jspackages/less/lib/less/tree/condition.jspackages/test-data/tests-unit/css-guards/css-guards.csspackages/test-data/tests-unit/css-guards/css-guards.less
| if (less && (less.render || less.parse)) { | ||
| lessPath = p; |
There was a problem hiding this comment.
Require render during compiler discovery to prevent runtime crashes.
At Line 46, discovery currently accepts compilers that only expose parse. But runOnce() always calls less.render (Line 93), so a parse-only module will fail at runtime with TypeError: less.render is not a function.
Proposed fix
- if (less && (less.render || less.parse)) {
+ if (less && typeof less.render === 'function') {
lessPath = p;
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (less && (less.render || less.parse)) { | |
| lessPath = p; | |
| if (less && typeof less.render === 'function') { | |
| lessPath = p; | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/less/benchmark/benchmark-runner.js` around lines 46 - 47, The
compiler discovery condition at line 46 currently uses OR logic to accept
compilers that have either `render` or `parse`, but the `runOnce()` function
always calls `less.render` at line 93. Change the condition from `(less.render
|| less.parse)` to `(less.render && less.parse)` to require both methods to be
present, preventing runtime crashes when a parse-only compiler is selected.
There was a problem hiding this comment.
Pull request overview
Adds support for the != inequality operator in Less guard conditions by extending both parsing and condition evaluation, and updates unit fixtures to validate expected CSS output. The PR also includes ancillary changes to build/benchmark tooling that are not directly related to the new operator.
Changes:
- Parse
!=in guard conditions (parser.js) and evaluate it inCondition.eval(condition.js). - Add guard fixtures verifying
!=behavior incss-guards.less/css. - Adjust build shim output string and refactor/format the benchmark runner.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/test-data/tests-unit/css-guards/css-guards.less | Adds new guard cases using != to validate behavior. |
| packages/test-data/tests-unit/css-guards/css-guards.css | Updates expected CSS output for the new != guard cases. |
| packages/less/lib/less/tree/condition.js | Implements evaluation semantics for != based on Node.compare results. |
| packages/less/lib/less/parser/parser.js | Extends condition parsing to recognize != as a comparison operator. |
| packages/less/build/rollup.js | Tweaks module shim string emission (scope/tooling change). |
| packages/less/benchmark/benchmark-runner.js | Refactors/cleans up benchmark runner logic/formatting (scope/tooling change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (e) { | ||
| // try next | ||
| } | ||
| } |
|
|
||
| // Standard deviation and coefficient of variation | ||
| var sumSqDiff = 0; | ||
| for (i = 0; i < effective.length; i++) { |
| .inequality-test-1 when (2 != 1) { | ||
| color: green; | ||
| } | ||
| .inequality-test-2 when (2 != 2) { | ||
| color: red; | ||
| } | ||
| .inequality-test-3 when (@b != 1) { | ||
| color: green; | ||
| } | ||
| .inequality-test-4 when (@b != 2) { | ||
| color: red; | ||
| } |
| }, | ||
| load(id) { | ||
| if (id === '\0module') { | ||
| return `export function createRequire() { return require; }`; | ||
| return 'export function createRequire() { return require; }'; | ||
| } |
|
Guard syntax / conditions is modelled on media query syntax. It doesn't have a |
|
Thanks for the explanation, Matthew! That makes complete sense. I hadn't fully considered the design alignment with CSS media query syntax when putting this together. Since not(a = b) is indeed the idiomatic way to handle this in Less and aligns with CSS standards, I completely understand closing this. Thanks again for your time and feedback! |
What:
!=) inpackages/less/lib/less/parser/parser.js.packages/less/lib/less/tree/condition.js:truewhen comparison results in inequality (-1or1).falsewhen comparison results in equality (0).truewhen comparison is undefined (types are not comparable, meaning they are not equal).packages/test-data/tests-unit/css-guards/css-guards.lessandcss-guards.css.Why:
Currently, Less lacks an inequality comparison operator, requiring users to write verbose guard checks like
not (@foo = 1)rather than the cleaner and standard@foo != 1. This aligns Less with standard CSS/JS and other CSS preprocessors.Checklist:
Summary by CodeRabbit
New Features
!=(not-equal) operator in guard conditions.Improvements
Tests